Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colorful + animated progress bars #215

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

melyux
Copy link
Contributor

@melyux melyux commented Aug 6, 2024

Re-implementation of the PR #82 from last year, on top of the recent change 05303b6. Adds the third possibility of using colorful + animated together (color based on status, animated based on activity):

image

Takes into account all of @qu1ck's suggestions from the original PR. Fits things to match the existing color scheme and the Transmission app:

  • blue for downloading/verifying/magnetizing
  • red for error
  • green if seeding
  • dark green if stopped @ 100%
  • yellow if stopped < 100% (to match the stopped icon's color)
  • gray if waiting
image

Only applies if user has both "colorful" and "animated" selected for progress bars in interface settings. Other combinations are unaffected by this.

@melyux melyux changed the title Implement colorful + animated progress bars Colorful + animated progress bars Aug 6, 2024
@Pentaphon
Copy link

I like this! Hope to see it in the next release.

@qu1ck
Copy link
Member

qu1ck commented Aug 8, 2024

I have yet to test this fully but I noticed one thing right away: you can't just remove a config option, you have to add logic to migrate it from old style enum to your checkboxes to not lose user preferences. There are some examples of this in config class, see openTabs.

And default should be the same: animated, not colorful.

Copy link
Member

@qu1ck qu1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this looks good, just one more thing to address:
There is a lot of common logic of determining the progress bar variant in torrenttable.tsx and details.tsx, extract that into a single util function, you can put it in utils.js.

@melyux
Copy link
Contributor Author

melyux commented Aug 10, 2024

@qu1ck For the deprecated setting in the Settings interface, I'm adding it back as a string since it's stored as a string in the settings JSON. This way, we can migrate the old setting but avoid having to keep its deprecated enum. Good?

        progressbarStyle?: string, // deprecated
        animatedProgressbars: boolean,
        colorfulProgressbars: boolean,

@qu1ck
Copy link
Member

qu1ck commented Aug 11, 2024

Yes, that's fine.

@qu1ck
Copy link
Member

qu1ck commented Aug 21, 2024

Friendly ping.
I'd like to cut a release soon and include your PRs in it.

@melyux
Copy link
Contributor Author

melyux commented Aug 27, 2024

Sure, was on vacation and back now. Halfway done with suggestions

@melyux
Copy link
Contributor Author

melyux commented Aug 28, 2024

@qu1ck Check out latest changes

Copy link
Member

@qu1ck qu1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the eslint issues please

src/components/details.tsx Outdated Show resolved Hide resolved
@melyux melyux force-pushed the colorful-animated-progressbars branch 2 times, most recently from 248a1cb to f2a28c0 Compare August 29, 2024 04:38
@melyux
Copy link
Contributor Author

melyux commented Aug 29, 2024

Done @qu1ck

@qu1ck qu1ck force-pushed the colorful-animated-progressbars branch from f2a28c0 to 1cd3d0c Compare August 29, 2024 07:15
@qu1ck
Copy link
Member

qu1ck commented Aug 29, 2024

Looks good, just one question. Was it intentional that when animations are disabled then torrents will be only blue/red/green? Why disable other colors?

@melyux
Copy link
Contributor Author

melyux commented Aug 29, 2024

I was conservative and just kept the original state of things for the original options, while adding a new combination option. Could expand the new colors to the "colorful" option if we're feeling frisky

@qu1ck
Copy link
Member

qu1ck commented Aug 29, 2024

Well in previous implementation things were either colorful or animated, they couldn't be both. But since this change has 2 different settings for both it makes sense that the difference between [colors on, animations off] and [colors on, animations on] would be only animations, the colors would stay the same.

@qu1ck
Copy link
Member

qu1ck commented Sep 11, 2024

@melyux can you at least comment if you are going to do change discussed above?
If you don't have time I can merge this as is and amend later.

@melyux
Copy link
Contributor Author

melyux commented Sep 11, 2024

Since you approved it as is already I thought it was for a future release. I can make the changes now then

@qu1ck
Copy link
Member

qu1ck commented Sep 11, 2024

Please do.
Sorry for the miscommunication and being ambiguous with the approval.

@melyux
Copy link
Contributor Author

melyux commented Sep 12, 2024

@qu1ck Done with the changes, but since you force pushed the branch (somehow!), it won't let me push again. How can I fix it?

@qu1ck
Copy link
Member

qu1ck commented Sep 12, 2024

I just rebased your branch without changes. You can force push again.

@melyux melyux force-pushed the colorful-animated-progressbars branch from 1cd3d0c to dedece5 Compare September 12, 2024 09:50
@melyux
Copy link
Contributor Author

melyux commented Sep 12, 2024

@qu1ck Done

@qu1ck qu1ck force-pushed the colorful-animated-progressbars branch from dedece5 to e6a29c9 Compare September 12, 2024 10:15
@qu1ck qu1ck merged commit 807041d into openscopeproject:master Sep 12, 2024
3 checks passed
@qu1ck
Copy link
Member

qu1ck commented Sep 12, 2024

Thanks for your contribution and patience. You still have one more PR open :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants